Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Update ajv to version 8 #1739

Merged
merged 2 commits into from
Oct 8, 2021
Merged

Update ajv to version 8 #1739

merged 2 commits into from
Oct 8, 2021

Conversation

AlexandraBuzila
Copy link
Member

@AlexandraBuzila AlexandraBuzila commented Apr 22, 2021 •
edited
Loading

Update to Angular 12

Update to Ajv 8

  • mitigate removal of option 'errorDataPath: property'. Use
    'instancePath' property and add new getControlPath method that returns a
    formatted path pointing to the invalid property and not to its parent.
    The errors from JsonFormsCore are now the original errors returned by
    Ajv
  • the error message for missing required properties was adapted to refer
    to the property and not its parent
  • remove custom time validation and use format validations from
    ajv-formats
  • replace 'jsonPointers: true' with 'jsPropertySyntax: true' ajv option
    as the former was removed
  • remove json schema draft 04 from default ajv instance
  • update imports and tests

@AlexandraBuzila AlexandraBuzila marked this pull request as draft April 22, 2021 16:07
sdirix
sdirix requested changes May 4, 2021
View reviewed changes
Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! I would prefer not touching the error objects themselves. When we emit them to the user they are not expecting them to be changed by JSON Forms. Instead we should identify the correct errors for our use cases in the mapStateToXYZProps methods.

@sdirix
Copy link
Member

sdirix commented Jun 24, 2021

In the mean time AJV 8 was released, so we should update to that instead ;)

@AlexandraBuzila AlexandraBuzila changed the title Update ajv to version 7 Update ajv to version 8 Aug 5, 2021
@sdirix sdirix mentioned this pull request Sep 7, 2021
Closed
@coveralls
Copy link

coveralls commented Sep 22, 2021 •
edited
Loading

Coverage Status

Coverage increased (+0.05%) to 89.042% when pulling fc6b037 on AlexandraBuzila:update-ajv into be5c82c on eclipsesource:master.

@Ketec
Copy link

Ketec commented Sep 30, 2021 •
edited
Loading

Cloned, and built the repo - as described in Readme.
Used angular jsonforms seed to test. Installed everything as default (angular 12.0.5 in lockfile).
Ran npm upgrade.
Moved the built jsonforms lib to the seed project and ran serve.

Major fatal errors (errors were there also before npm upgrade):
Cannot use namespace 'Ajv' as a type. Type 'ErrorObject' is not generic.

@sdirix
Copy link
Member

sdirix commented Sep 30, 2021

@Ketec Thank you very much for taking a look!

Cloned, and built the repo - as described in Readme. Used angular jsonforms seed to test. Installed everything as default (angular 12.0.5 in lockfile). Ran npm upgrade. Moved to build jsonforms lib to seed project and ran serve.

Major fatal errors (errors were there also before npm upgrade): Cannot use namespace 'Ajv' as a type. Type 'ErrorObject' is not generic.

Is the error originating from the seed code, i.e. app.component.ts? If yes I'm pretty sure that the code needs to be adapted to the new AJV version.

@Ketec
Copy link

Ketec commented Sep 30, 2021 •
edited
Loading

@sdirix It is all over the place - core lib utils, reducers, actions. Angular lib root component

Error: node_modules/@jsonforms/angular/lib/jsonforms-root.component.d.ts:20:10 - error TS2709: Cannot use namespace 'Ajv' as a type.

20     ajv: Ajv;
            ~~~


Error: node_modules/@jsonforms/angular/lib/jsonforms-root.component.d.ts:22:26 - error TS2315: Type 'ErrorObject' is not generic.

22     errors: EventEmitter<ErrorObject<string, Record<string, any>, unknown>[]>;
                            ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

@sdirix
Copy link
Member

sdirix commented Sep 30, 2021

Which AJV version is in your node_modules directory?

@Ketec
Copy link

Ketec commented Oct 1, 2021

Ok, the seed itself was out of date - it was upgraded to Angular 12 but not AJV 8.

  ajv = createAjv({
    schemaId: 'id',
    allErrors: true
  });

schemaId / jsonPointers / errorDataPath - had to clean up the options for it to compile and run.

sdirix
sdirix reviewed Oct 4, 2021
View reviewed changes
Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How much effort do we need to get rid off the already deprecated option?

packages/core/src/util/validator.ts Outdated Show resolved Hide resolved
@sdirix sdirix marked this pull request as ready for review October 4, 2021 15:54
sdirix
sdirix requested changes Oct 6, 2021
View reviewed changes
Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

With the AJV update we are now properly validating format: 'time'. It seems that our React Material Time renderer produces an invalid format, leading to always invalid inputs.

Please update the time renderer to store the time in a proper output by default. If a user overwrites the saveFormat and therefore it becomes invalid then that is fine. You can reproduce the issue in the control options example.

@AlexandraBuzila @sdirix
Update to Ajv v8
fc6b037 
Ajv 8 removes the 'errorDataPath' property option. Error code is
refactored to handle the new Ajv error objects without adapting them.
The errors stored in the state are now the original errors returned by
Ajv.

Also removes custom time validation and uses standardized format
validations from ajv-formats instead. Additionally the JSON Schema v4
format is no longer added by default.

Updates imports and tests.
sdirix
sdirix approved these changes Oct 8, 2021
View reviewed changes
Copy link
Member

@sdirix sdirix left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you very much! Looks good to me!

@sdirix sdirix merged commit 3f706fe into eclipsesource:master Oct 8, 2021
This was linked to issues Oct 8, 2021
Bump angular packages to v12 #1801
Closed
Update to Ajv v7 #1688
Closed
@sdirix sdirix linked an issue Oct 8, 2021 that may be closed by this pull request
Ajv deprecated "errorDataPath" option. #1565
Closed
@sdirix sdirix mentioned this pull request Nov 8, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@sdirix sdirix sdirix approved these changes

Assignees

@AlexandraBuzila AlexandraBuzila

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

Bump angular packages to v12 Update to Ajv v7 Ajv deprecated "errorDataPath" option.
4 participants
@AlexandraBuzila @sdirix @coveralls @Ketec